Skip to content

Comments

[Metricbeat] Add reporting interface with error#10727

Merged
ruflin merged 14 commits intoelastic:masterfrom
ruflin:reporter-error-interface
Mar 6, 2019
Merged

[Metricbeat] Add reporting interface with error#10727
ruflin merged 14 commits intoelastic:masterfrom
ruflin:reporter-error-interface

Conversation

@ruflin
Copy link
Contributor

@ruflin ruflin commented Feb 13, 2019 •
edited
Loading

In Metricbeat modules we often see the pattern that before fetching the actual data, some processing is done and checks for errors happen. If an error happened, we have 3 lines of code:

  • creating the error with wrap
  • reporting the error
  • logging the error

By introducing the reporter interface with support for return an error I would like to eliminate the overhead and allow to directly return and error which is then reported and also logged.

So far we logged the error on the Error level. I'm now wondering if we should log these errors actually on the Info level as it's normally not a misbehaving of the Beat but the service does not respond as expected. So for the operator of the Beat normally no actions are needed.

As an example metricset I took php_fpm.process.

@ruflin ruflin added discuss Issue needs further discussion. review Metricbeat Metricbeat technical_debt labels Feb 13, 2019
@ruflin ruflin requested review from a team as code owners February 13, 2019 13:56
houndci-bot
houndci-bot reviewed Feb 13, 2019
View reviewed changes
@ruflin ruflin force-pushed the reporter-error-interface branch from db23d86 to 1d03797 Compare February 13, 2019 13:57
cachedout
cachedout reviewed Feb 13, 2019
View reviewed changes
Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot! I was just wondering about this last night so I am very happy to see it.

ycombinator
ycombinator reviewed Feb 13, 2019
View reviewed changes
@ycombinator
Copy link
Contributor

ycombinator commented Feb 13, 2019

So far we logged the error on the Error level. I'm now wondering if we should log these errors actually on the Info level as it's normally not a misbehaving of the Beat but the service does not respond as expected. So for the operator of the Beat normally no actions are needed.

I'm still in favor of logging errors at the ERROR level. From a debugging perspective, when looking at the Metricbeat logs, it makes it really easy to spot errors. Then, from the error, we can figure out if it's a service error or a beat error. My concern with dropping service errors down to INFO level is that they'll get lost in the log noise pretty easily, making debugging harder.

@kaiyan-sheng
Copy link
Contributor

kaiyan-sheng commented Feb 13, 2019

Maybe adding a log level as input parameter so we can set to ERROR or INFO depends on the each specific case?

sayden
sayden approved these changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea!

@ruflin
Copy link
Contributor Author

ruflin commented Feb 14, 2019

For the logging level, have a look at my longer comment here: #10727 (comment) I think error logging should be focused on the production and not the setup / testing use case.

@ruflin ruflin force-pushed the reporter-error-interface branch 2 times, most recently from bf89bbd to c6d1ada Compare February 14, 2019 12:51
kaiyan-sheng
kaiyan-sheng reviewed Feb 15, 2019
View reviewed changes
@ruflin ruflin force-pushed the reporter-error-interface branch from c6d1ada to 9c09c1c Compare February 15, 2019 20:53
houndci-bot
houndci-bot reviewed Feb 15, 2019
View reviewed changes
andrewkroh
andrewkroh reviewed Feb 16, 2019
View reviewed changes
jsoriano
jsoriano approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd try to avoid adding a new fetcher interface, but as we can still reuse ReporterV2 and it is so easy to implement from the ReportingMetricSetV2 one I am not going to complain 🙂

@sayden sayden mentioned this pull request Feb 20, 2019
Closed
62 tasks
@ruflin ruflin force-pushed the reporter-error-interface branch from 1b91a13 to cd2bf04 Compare March 4, 2019 13:51
houndci-bot
houndci-bot reviewed Mar 5, 2019
View reviewed changes
@ruflin ruflin mentioned this pull request Mar 5, 2019
Closed
@ruflin ruflin removed the discuss Issue needs further discussion. label Mar 5, 2019
@ruflin ruflin force-pushed the reporter-error-interface branch from a86ffb4 to 681f41a Compare March 6, 2019 13:58
@ruflin ruflin merged commit 45074a3 into elastic:master Mar 6, 2019
@ruflin ruflin deleted the reporter-error-interface branch March 6, 2019 15:04
@fearful-symmetry fearful-symmetry mentioned this pull request Mar 6, 2019
Closed
10 tasks
This was referenced Mar 21, 2019
Closed
Merged
Merged
This was referenced Mar 23, 2019
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Apr 11, 2019
Merged
Merged
Merged
ycombinator added a commit that referenced this pull request Apr 12, 2019
@ycombinator
Refactoring: use new Fetch interface that automatically reports and l…
462f798 
…ogs errors (#11763)

Refactors code in the `kibana` Metricbeat module to use the new `Fetch` interface introduced in #10727.

Note that x-pack code paths in this module were not refactored to use the new interface as we don't want errors from those code paths to be reported into `metricbeat-*` indices, only logged to Metricbeat logs.

Related: #11767.
@ycombinator ycombinator mentioned this pull request Apr 12, 2019
Merged
@ycombinator ycombinator mentioned this pull request May 28, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ycombinator ycombinator ycombinator left review comments

@cachedout cachedout cachedout left review comments

@andrewkroh andrewkroh andrewkroh left review comments

@kaiyan-sheng kaiyan-sheng kaiyan-sheng left review comments

@jsoriano jsoriano jsoriano approved these changes

+2 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@sayden sayden sayden approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Metricbeat Metricbeat review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ruflin @ycombinator @kaiyan-sheng @jsoriano @cachedout @sayden @andrewkroh @houndci-bot